Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Radar Chart Span Gaps #5359

Closed
wants to merge 15 commits into from
Closed

Radar Chart Span Gaps #5359

wants to merge 15 commits into from

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Mar 22, 2018 •
edited
Loading

Resolves #5358

To Do

  • Tests
  • Docs
  • Code cleanup

@etimberg etimberg mentioned this pull request Mar 22, 2018
Closed
@etimberg
Copy link
Member Author

@simonbrunel I added tests and a section to the docs. I found bugs in the line element and the filler plugin when the first point in the line is skipped. This PR includes fixes for those as well

simonbrunel
simonbrunel reviewed Apr 2, 2018
View reviewed changes
src/controllers/controller.radar.js Outdated
@@ -50,6 +51,7 @@ module.exports = function(Chart) {
// Model
_model: {
// Appearance
spanGaps: dataset.spanGaps ? dataset.spanGaps : me.chart.options.spanGaps,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataset.spanGaps !== undefined ? dataset.spanGaps : me.chart.options.spanGaps, else it will ignore dataset.spanGaps: false

src/controllers/controller.radar.js Outdated

for (i = 0, ilen = points.length; i < ilen; ++i) {
point = points[i];
model = point._model;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model = points[i]._model;

benmccann
benmccann reviewed Apr 15, 2018
View reviewed changes
src/controllers/controller.radar.js Outdated
@@ -50,6 +51,7 @@ module.exports = function(Chart) {
// Model
_model: {
// Appearance
spanGaps: dataset.spanGaps !== undefined ? dataset.spanGaps : me.chart.options.spanGaps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could do:

spanGaps: helpers.valueOrDefault(dataset.spanGaps, me.chart.options.spanGaps)

benmccann
benmccann reviewed Apr 15, 2018
View reviewed changes
src/controllers/controller.radar.js Outdated
helpers.each(points, function(point, index) {
me.updateElement(point, index, reset);
}, me);
for (i = 0, ilen = points.length; i < ilen; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++i is a little non-standard. The file has i++ everywhere else, which I see much more commonly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using ++i in for loops but it's an habit that comes from C/C++ which is (was?) an optimization. I'm not sure it has any incidence in JavaScript though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v8 produces the same bytecode in either case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The habit for me came from my c++ days too. Will try and address the comments tomorrow

benmccann
benmccann reviewed Apr 15, 2018
View reviewed changes
src/elements/element.line.js Outdated
if (me._loop && points.length) {
points.push(points[0]);
for (index = 0; index < points.length; ++ index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an extra space in ++ index (also I think index++ would be more consistent)

@benmccann benmccann mentioned this pull request Apr 22, 2018
Closed
@benmccann
Copy link
Contributor

@etimberg don't forget there's a few comments on this PR

@benmccann
Copy link
Contributor

@etimberg just a quick reminder about this PR

benmccann
benmccann previously approved these changes May 21, 2018
View reviewed changes
@simonbrunel
Copy link
Member

@etimberg the CI is failing, can you have a look?

@etimberg
Copy link
Member Author

@simonbrunel I fixed the linting error. Will try and investigate the test failures. They don't happen locally but I thought I'd gotten all the text off

@benmccann benmccann mentioned this pull request Jul 8, 2018
Merged
@benmccann
Copy link
Contributor

@etimberg one of the failing tests "Chart.elements.Line should be able to draw with a loop back to the beginning point when the first point is skipped" is here: https://github.com/chartjs/Chart.js/blob/master/test/specs/element.line.tests.js#L1876

With your changes there's an extra lineTo:

9: {name: "lineTo", args: [15, -10] }
10: {name: "lineTo", args: [19, -5] }
11: {name: "lineTo", args: [5, 0] }

Do you need to update element.line.tests to include an extra lineTo like this? benmccann@2454c41

Or is it a bug that there's an additional lineTo being included?

@benmccann
Copy link
Contributor

@etimberg the other test that's failing are the radar image tests below. I've included these screenshots to help demonstrate how exactly it's failing

screenshot from 2018-07-08 18-50-53

They seem to be caused by the change to:

		if (value === null || isNaN(value)) {
			return NaN; // null always in center
		}

If I change return NaN to return 0 then the tests pass

@etimberg
Copy link
Member Author

etimberg commented Jul 9, 2018

Thanks for the images @benmccann. Going to look into this now.

@etimberg
Copy link
Member Author

Closing since this surprisingly hard to get right. The radar chart should skip null points but doing so breaks the filler plugin.

@etimberg etimberg closed this Jul 29, 2018
@nagix nagix mentioned this pull request May 22, 2019
Merged
@etimberg etimberg deleted the radar-chart-span-gaps branch October 27, 2019 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbrunel simonbrunel simonbrunel left review comments

@benmccann benmccann benmccann left review comments

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@etimberg @benmccann @simonbrunel